Skip to content

fix Problem with displaying whitespace chars literals #1975 #1976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
asukaminato0721 wants to merge 1 commit into from
Closed

fix Problem with displaying whitespace chars literals #1975 #1976

asukaminato0721 wants to merge 1 commit into from
+16 −5

Conversation

@asukaminato0721
Copy link
Contributor

@asukaminato0721 asukaminato0721 commented Jan 1, 2026

Summary

Fixes #1975

Adjusted byte literal display to escape control characters, quotes, and backslashes so defaults render as readable escapes instead of raw whitespace, and added a regression test for whitespace/control bytes.

Test Plan

add a literal-bytes display assertion covering whitespace/control bytes.

Copilot AI review requested due to automatic review settings January 1, 2026 14:47
@meta-cla meta-cla bot added the cla signed label Jan 1, 2026
Copilot AI reviewed Jan 1, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the display of whitespace and control characters in bytes literals by properly escaping them instead of rendering them as raw characters. Previously, control characters like tabs and newlines would be displayed literally, causing readability issues.

Key Changes:

  • Replaced Unicode character conversion with explicit byte-level escape handling for bytes literals
  • Added proper escaping for common control characters (\t, \n, \r) and special characters (\\, \')
  • Added regression test to verify whitespace and control bytes are displayed correctly

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/pyrefly_types/src/literal.rs Removed unused char import and replaced char::from_u32 conversion with pattern matching to properly escape control characters, quotes, and backslashes in bytes literals
crates/pyrefly_types/src/display.rs Added test case verifying that whitespace and control characters in bytes literals are properly escaped in display output

After thoroughly reviewing the changes, I found no issues with this PR. The implementation correctly:

  • Escapes common control characters (\t, \n, \r)
  • Escapes backslashes and single quotes (necessary for single-quoted bytes literals)
  • Renders printable ASCII characters (space through tilde) as-is
  • Escapes all other bytes using hex notation (\xNN)
  • Removes the now-unused char import
  • Includes an appropriate regression test

The changes align with Python's bytes literal syntax and fix the reported issue with whitespace character display.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@guoci
Copy link

guoci commented Jan 2, 2026

@asukaminato0721 Would it be better to reuse the code for string literals?

pyrefly/crates/pyrefly_types/src/literal.rs

Lines 180 to 209 in 51ea20d

pub fn to_string_escaped(&self, use_single_quotes_for_string: bool) -> String {
match self {
Lit::Str(s) => {
let escaped: String = s
.chars()
.map(|c| match c {
// https://docs.python.org/3/reference/lexical_analysis.html#escape-sequences
'\\' => "\\\\".to_owned(),
'\'' if use_single_quotes_for_string => "\\'".to_owned(),
'\"' if !use_single_quotes_for_string => "\\\"".to_owned(),
'\x07' => "\\a".to_owned(), // \a
'\x08' => "\\b".to_owned(), // \b
'\x0c' => "\\f".to_owned(), // \f
'\n' => "\\n".to_owned(),
'\r' => "\\r".to_owned(),
'\t' => "\\t".to_owned(),
'\x0b' => "\\v".to_owned(), // \v
_ => c.to_string(),
})
.collect::<String>();
if use_single_quotes_for_string {
format!("'{escaped}'")
} else {
format!("\"{escaped}\"")
}
}
lit => lit.to_string(),
}
}
}

@asukaminato0721
Copy link
Contributor Author

asukaminato0721 commented Jan 2, 2026 •
edited
Loading

it's a bit different

Lit::Bytes(bytes) => {

vs

Lit::Str(s) => {

the match arm is char vs b' '

@meta-codesync
Copy link

meta-codesync bot commented Jan 5, 2026

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D90125148.

@kinto0 kinto0 self-assigned this Jan 5, 2026
stroxler
stroxler approved these changes Jan 5, 2026
View reviewed changes
Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

@meta-codesync meta-codesync bot closed this in a68b0d1 Jan 5, 2026
@meta-codesync
Copy link

meta-codesync bot commented Jan 5, 2026

@kinto0 merged this pull request in a68b0d1.

@asukaminato0721 asukaminato0721 deleted the 1975 branch January 6, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@stroxler stroxler stroxler approved these changes

Assignees

@kinto0 kinto0

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Problem with displaying whitespace chars literals

5 participants

@asukaminato0721 @guoci @stroxler @facebook-github-bot @kinto0